[RORDEV-1414] ES node details audit reporting #1116
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # core/src/test/scala/tech/beshu/ror/utils/TestsUtils.scala # gradle.properties
# Conflicts: # gradle.properties
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
coutoPL
requested changes
May 19, 2025
...n/scala/tech/beshu/ror/accesscontrol/audit/AuditLogSerializerEnrichedWithEsNodeDetails.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/tech/beshu/ror/accesscontrol/audit/AuditingTool.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/tech/beshu/ror/accesscontrol/audit/AuditingTool.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/tech/beshu/ror/configuration/ReadonlyRestEsConfig.scala
Outdated
Show resolved
Hide resolved
es67x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala
Outdated
Show resolved
Hide resolved
# Conflicts: # core/src/main/scala/tech/beshu/ror/accesscontrol/audit/AuditingTool.scala # core/src/main/scala/tech/beshu/ror/boot/ReadonlyRest.scala # core/src/test/scala/tech/beshu/ror/integration/AuditOutputFormatTests.scala # core/src/test/scala/tech/beshu/ror/unit/acl/logging/AuditingToolTests.scala # core/src/test/scala/tech/beshu/ror/unit/boot/ReadonlyRestStartingTests.scala # core/src/test/scala/tech/beshu/ror/unit/boot/RorIndexTest.scala # core/src/test/scala/tech/beshu/ror/utils/TestsUtils.scala # es67x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es67x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es70x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es70x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es710x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es710x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es711x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es711x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es714x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es714x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es716x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es716x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es717x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es717x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es72x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es72x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es73x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es73x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es74x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es74x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es77x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es77x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es78x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es78x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es79x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es79x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es80x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es80x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es810x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es810x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es811x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es811x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es812x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es812x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es813x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es813x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es814x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es814x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es815x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es815x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es816x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es816x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es81x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es81x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es82x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es82x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es83x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es83x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es84x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es84x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es85x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es85x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es87x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es87x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es88x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es88x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es89x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es89x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # es90x/src/main/scala/tech/beshu/ror/es/IndexLevelActionFilter.scala # es90x/src/main/scala/tech/beshu/ror/es/ReadonlyRestPlugin.scala # gradle.properties # integration-tests/src/test/scala/tech/beshu/ror/integration/suites/audit/LocalClusterAuditingToolsSuite.scala # integration-tests/src/test/scala/tech/beshu/ror/integration/suites/audit/RemoteClusterAuditingToolsSuite.scala # integration-tests/src/test/scala/tech/beshu/ror/integration/suites/base/BaseAuditingToolsSuite.scala
mgoworko
force-pushed
the
RORDEV-1414-es-node-details-reporting
branch
from
1169b5a to
50062d9
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
coutoPL
reviewed
May 25, 2025
.../main/scala/tech/beshu/ror/audit/enrichers/AuditLogSerializerEnrichedWithEsNodeDetails.scala
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
coutoPL
reviewed
Jun 13, 2025
| @@ -19,5 +19,6 @@ package tech.beshu.ror.audit | |||
| import org.json.JSONObject | |||
|
|
|||
| trait AuditLogSerializer { | |||
| def onResponse(responseContext: AuditResponseContext): Option[JSONObject] | |||
| def onResponse(responseContext: AuditResponseContext, | |||
There was a problem hiding this comment.
What about backward compatibility here? The existing custom serializers should work with the new approach too.
| whenEnabled(c) { | ||
| for { | ||
| auditIndexTemplate <- decodeOptionalSetting[RorAuditIndexTemplate](c)("index_template", fallbackKey = "audit_index_template") | ||
| customAuditSerializer <- decodeOptionalSetting[AuditLogSerializer](c)("serializer", fallbackKey = "audit_serializer") | ||
| remoteAuditCluster <- decodeOptionalSetting[AuditCluster.RemoteAuditCluster](c)("cluster", fallbackKey = "audit_cluster") | ||
| } yield AuditingTool.Settings( | ||
| enableReportingEsNodeDetails <- c.downField("enable_reporting_es_node_details").as[Option[Boolean]] |
There was a problem hiding this comment.
Do we leave the toggle? Why?
| */ | ||
| package tech.beshu.ror.es | ||
|
|
||
| case class EsNodeSettings(nodeName: String, clusterName: String) |
| esVersion = EsVersion(major = Version.CURRENT.major, minor = Version.CURRENT.minor, revision = Version.CURRENT.revision) | ||
| esVersion = EsVersion(major = Version.CURRENT.major, minor = Version.CURRENT.minor, revision = Version.CURRENT.revision), | ||
| esNodeSettings = EsNodeSettings( | ||
| nodeName = settings.get("node.name"), |
There was a problem hiding this comment.
Do you confirm these two will always be non-null? Even if the user doesn't set them? (I'm not sure if they are required settings)
gradle.properties
Outdated
| @@ -1,5 +1,5 @@ | |||
| publishedPluginVersion=1.64.2 | |||
| pluginVersion=1.65.0-pre1 | |||
| pluginVersion=1.65.0-pre2 | |||
| @@ -61,4 +62,10 @@ class RemoteClusterAuditingToolsSuite | |||
| override protected def baseRorConfig: String = resolvedRorConfigFile.contentAsString | |||
|
|
|||
| override protected def baseAuditDataStreamName: Option[String] = Option.when(isDataStreamSupported)("audit_data_stream") | |||
|
|
|||
| // Adding the ES cluster fields is enabled in the /cluster_auditing_tools/readonlyrest.yml config file (`DefaultAuditLogSerializerV2` is used) | |||
| override def assertForEveryAuditEntry(entry: JSON): Unit = { | |||
There was a problem hiding this comment.
I'm not sure if this test covers this use case too, but I'd be nice to test the scenario as above:
1. The user starts ES with ROR with `DefaultAuditLogSerializerV1` configured
2. The user generates some traffic to create some audit documents
3. The user reconfigures ROR to use `DefaultAuditLogSerializerV2` and reloads config
4. The user generates some traffic to create some audit documents
The result - there should be no problem with the scenario above.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/AuditingSettingsDecoder.scala (1)
80-90:distincton outputs hides duplicates silentlySilently deduping outputs turns a configuration error into “last one wins”, potentially confusing operators. Prefer failing fast:
- .fromList(outputs.distinct) + .fromList(outputs) .map(AuditingTool.AuditSettings.apply)Then, before construction, detect duplicates and return
AuditingSettingsCreationError.
♻️ Duplicate comments (2)
es88x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala (1)
24-34: Same duplication remark applies herees87x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala (1)
24-34: Same duplication remark applies here
🧹 Nitpick comments (6)
es89x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala (1)
24-34: Code duplication across EsEnvProvider versionsThe body of
createis now almost identical ines87x,es88x,es89x,es90x(onlyconfigDirvsconfigFile). Consider extracting the common logic into a shared helper/trait to keep behaviour in-sync and reduce maintenance overhead.Example sketch:
private[utils] trait CommonEsEnvProvider { protected def buildEnv(configPath: Path, modulesPath: Path, environment: Environment): EsEnv = { val settings = environment.settings() EsEnv( configPath = configPath, modulesPath = modulesPath, esVersion = EsVersion(Version.CURRENT), esNodeSettings = EsNodeSettings( nodeName = Node.NODE_NAME_SETTING.get(settings), clusterName = ClusterName.CLUSTER_NAME_SETTING.get(settings) ) ) } }Each version-specific object would just call
buildEnv(env.configFile(), env.modulesFile(), env).audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializer.scala (1)
19-21: Consider clarifying intent of the thin wrapper
DefaultAuditLogSerializernow only delegates toDefaultAuditLogSerializerV2. If the old class exists solely for binary compatibility, mark it with@deprecated("Use DefaultAuditLogSerializerV2", "x.y.z")andfinalto prevent further subclassing. Otherwise, the indirection looks redundant.-class DefaultAuditLogSerializer(environmentContext: AuditEnvironmentContext) - extends DefaultAuditLogSerializerV2(environmentContext) +@deprecated("Replaced by DefaultAuditLogSerializerV2", "3.0.0") +final class DefaultAuditLogSerializer(environmentContext: AuditEnvironmentContext) + extends DefaultAuditLogSerializerV2(environmentContext)Keeps API consumers informed and the public surface tidy.
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/RawRorConfigBasedCoreFactory.scala (1)
321-326: Avoid re-allocatingAuditEnvironmentContextinside every decode
auditEnvironmentContextis recomputed for every invocation of the decoder, even though it is a pure value derived from the immutableesEnv.- auditEnvironmentContext = AuditEnvironmentContext( - esNodeName = esEnv.esNodeSettings.nodeName, - esClusterName = esEnv.esNodeSettings.clusterName - ) + // compute once, outside the decoder + auditEnvironmentContext = precomputedAuditEnvCtxCreating it once (e.g. a
val precomputedAuditEnvCtxdefined right beforecoreDecoder) reduces clutter and avoids needless allocations on hot paths.audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV2.scala (1)
24-31: Inline underscore infoldLefthurts readabilityThe partially-applied underscore trick works, but most devs need a second glance. An explicit lambda reads clearer and allows the compiler to infer types without surprises:
- super.onResponse(responseContext) - .map(additionalFields.foldLeft(_) { case (soFar, (key, value)) => soFar.put(key, value) }) + super.onResponse(responseContext).map { json => + additionalFields.foldLeft(json) { case (acc, (k, v)) => acc.put(k, v) } + }Same behaviour, less “Scala golf”.
audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV1.scala (1)
66-67:content_len_kbloses precision for sub-kilobyte payloadsInteger division by 1024 yields
0for anything<1 KiB, which can be misleading in dashboards. Consider emitting a decimal (e.g.content_len_bytes / 1024.0) or simply drop the derived field and let downstream tooling handle unit conversion.core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/AuditingSettingsDecoder.scala (1)
61-70: Unnecessary explicitsummon[AuditEnvironmentContext]
decodeAuditSettingsalready has the context in scope via the contextualusingclause. Passing it explicitly is redundant and slightly obscures intent.- decodeAuditSettings(using esVersion, summon[AuditEnvironmentContext])(c).map(Some.apply) + decodeAuditSettings(using esVersion)(c).map(Some.apply)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
audit/src/main/scala/tech/beshu/ror/audit/adapters/DeprecatedAuditLogSerializerAdapter.scala(1 hunks)audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializer.scala(1 hunks)audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV1.scala(1 hunks)audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV2.scala(1 hunks)audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializer.scala(1 hunks)audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV1.scala(1 hunks)audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV2.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/audit/AuditingTool.scala(8 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/RawRorConfigBasedCoreFactory.scala(3 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/AuditingSettingsDecoder.scala(9 hunks)core/src/main/scala/tech/beshu/ror/boot/ReadonlyRest.scala(1 hunks)core/src/main/scala/tech/beshu/ror/es/EsNodeSettings.scala(1 hunks)es67x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es70x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es710x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es711x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es714x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es716x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es717x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es72x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es73x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es74x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es77x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es78x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es79x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es80x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es810x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es811x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es812x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es813x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es814x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es815x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es816x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es818x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es81x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es82x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es83x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es84x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es85x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es87x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es88x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es89x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)es90x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala(1 hunks)gradle.properties(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- audit/src/main/scala/tech/beshu/ror/audit/adapters/DeprecatedAuditLogSerializerAdapter.scala
🚧 Files skipped from review as they are similar to previous changes (34)
- gradle.properties
- es79x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- core/src/main/scala/tech/beshu/ror/es/EsNodeSettings.scala
- audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV2.scala
- es67x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- core/src/main/scala/tech/beshu/ror/boot/ReadonlyRest.scala
- es81x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es815x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es711x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es818x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es810x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es84x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es70x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es716x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es814x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es78x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es816x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es73x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es717x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es710x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es74x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV1.scala
- es714x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializer.scala
- es83x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es812x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es813x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es85x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es811x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es80x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- core/src/main/scala/tech/beshu/ror/accesscontrol/audit/AuditingTool.scala
- es77x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es82x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
- es72x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala
🔇 Additional comments (2)
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/RawRorConfigBasedCoreFactory.scala (1)
71-73: Constructor now receivesEsEnv– good alignment with wider refactorPassing the whole
EsEnvinstead of justEsVersionunblocks future extensions without re-touching this factory.core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/AuditingSettingsDecoder.scala (1)
45-47: Good: implicitAuditEnvironmentContextnow requiredThe new signature enforces environment propagation at compile-time – a welcome safety net.
Comment on lines
+25
to
+33
| val settings = environment.settings() | ||
| EsEnv( | ||
| configPath = environment.configDir(), | ||
| modulesPath = environment.modulesDir(), | ||
| esVersion = EsVersion(major = Version.CURRENT.major, minor = Version.CURRENT.minor, revision = Version.CURRENT.revision) | ||
| esVersion = EsVersion(major = Version.CURRENT.major, minor = Version.CURRENT.minor, revision = Version.CURRENT.revision), | ||
| esNodeSettings = EsNodeSettings( | ||
| nodeName = settings.get("node.name", "elasticsearch"), | ||
| clusterName = settings.get("cluster.name", "elasticsearch"), | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use ES-provided constants instead of raw setting keys
settings.get("node.name", …) and friends work, but Elasticsearch already exposes these as typed settings (Node.NODE_NAME_SETTING, ClusterName.CLUSTER_NAME_SETTING, etc.). Using them avoids typos, buys compile-time safety and automatically inherits any default value changes introduced by ES.
-import tech.beshu.ror.es.{EsEnv, EsNodeSettings, EsVersion}
+import tech.beshu.ror.es.{EsEnv, EsNodeSettings, EsVersion}
+import org.elasticsearch.node.Node
+import org.elasticsearch.cluster.ClusterName
…
- esNodeSettings = EsNodeSettings(
- nodeName = settings.get("node.name", "elasticsearch"),
- clusterName = settings.get("cluster.name", "elasticsearch"),
- ),
+ esNodeSettings = EsNodeSettings(
+ nodeName = Node.NODE_NAME_SETTING.get(settings),
+ clusterName = ClusterName.CLUSTER_NAME_SETTING.get(settings),
+ ),Besides safety, this makes future version bumps less brittle.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val settings = environment.settings() | |
| EsEnv( | |
| configPath = environment.configDir(), | |
| modulesPath = environment.modulesDir(), | |
| esVersion = EsVersion(major = Version.CURRENT.major, minor = Version.CURRENT.minor, revision = Version.CURRENT.revision) | |
| esVersion = EsVersion(major = Version.CURRENT.major, minor = Version.CURRENT.minor, revision = Version.CURRENT.revision), | |
| esNodeSettings = EsNodeSettings( | |
| nodeName = settings.get("node.name", "elasticsearch"), | |
| clusterName = settings.get("cluster.name", "elasticsearch"), | |
| ), | |
| // … other imports … | |
| import tech.beshu.ror.es.{EsEnv, EsNodeSettings, EsVersion} | |
| +import org.elasticsearch.node.Node | |
| +import org.elasticsearch.cluster.ClusterName | |
| // … inside your method … | |
| val settings = environment.settings() | |
| EsEnv( | |
| configPath = environment.configDir(), | |
| modulesPath = environment.modulesDir(), | |
| esVersion = EsVersion( | |
| major = Version.CURRENT.major, | |
| minor = Version.CURRENT.minor, | |
| revision = Version.CURRENT.revision | |
| ), | |
| esNodeSettings = EsNodeSettings( | |
| - nodeName = settings.get("node.name", "elasticsearch"), | |
| - clusterName = settings.get("cluster.name", "elasticsearch"), | |
| + nodeName = Node.NODE_NAME_SETTING.get(settings), | |
| + clusterName = ClusterName.CLUSTER_NAME_SETTING.get(settings), | |
| ), | |
| ) |
🤖 Prompt for AI Agents
In es90x/src/main/scala/tech/beshu/ror/es/utils/EsEnvProvider.scala around lines
25 to 33, replace the raw string keys like "node.name" and "cluster.name" used
in settings.get() with the corresponding Elasticsearch provided constants such
as Node.NODE_NAME_SETTING and ClusterName.CLUSTER_NAME_SETTING. This change
improves type safety, prevents typos, and ensures default values stay consistent
with Elasticsearch updates. Update the imports if necessary to include these
constants.
Comment on lines
208
to
+223
| @nowarn("cat=deprecation") | ||
| private given Decoder[AuditLogSerializer] = | ||
| private given customAuditLogSerializer(using AuditEnvironmentContext): Decoder[AuditLogSerializer] = | ||
| SyncDecoderCreator | ||
| .from(Decoder.decodeString) | ||
| .emapE { fullClassName => | ||
| val serializer = Try(Class.forName(fullClassName).getDeclaredConstructor()).map(constructor => constructor.newInstance()) | ||
| .orElse { | ||
| Try(Class.forName(fullClassName).getConstructor(classOf[AuditEnvironmentContext])) | ||
| .map(constructor => constructor.newInstance(summon[AuditEnvironmentContext])) | ||
| } | ||
| .getOrElse( | ||
| throw new IllegalStateException( | ||
| s"Class ${Class.forName(fullClassName).getName} is required to have either one (AuditEnvironmentContext) parameter constructor or constructor without parameters" | ||
| ) | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reflection constructor detection order may discard env context
You attempt the no-arg constructor first; if present, a serializer that actually needs AuditEnvironmentContext will be instantiated without it, losing node/cluster data with no warning.
Swap the order and remove the extra Class.forName calls to cut reflection overhead:
- val serializer = Try(Class.forName(fullClassName).getConstructor(classOf[AuditEnvironmentContext]))
- .map(_.newInstance(summon[AuditEnvironmentContext]))
- .orElse {
- Try(Class.forName(fullClassName).getDeclaredConstructor()).map(_.newInstance())
- }
+ val clazz = Class.forName(fullClassName)
+ val serializer =
+ Try(clazz.getConstructor(classOf[AuditEnvironmentContext]))
+ .map(_.newInstance(summon[AuditEnvironmentContext]))
+ .orElse(Try(clazz.getDeclaredConstructor()).map(_.newInstance()))
.getOrElse(This guarantees serializers that expect the context receive it.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/AuditingSettingsDecoder.scala
between lines 208 and 223, the current code tries to instantiate the serializer
using the no-argument constructor first, which can cause serializers requiring
AuditEnvironmentContext to be created without it, losing important context. To
fix this, reverse the order of constructor detection to try the constructor with
AuditEnvironmentContext first, then fallback to the no-argument constructor.
Also, optimize by calling Class.forName only once and reuse the resulting Class
object to reduce reflection overhead.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🚀New (ES) Added ES node name and cluster name in the ES cluster audit reporting
Summary by CodeRabbit